Utils: Spread objects in cloneSceneObjectState #967
Conversation
kaydelaney
added
type/bug
|
where do we mutate the queries object? |
|
@torkelo It might be something strange some datasources do - could only reproduce with graphite/Mock datasource but not prometheus for example. I'll check. |
|
Okay so looking into a bit more, at least in the case of Graphite, I think what's happening is it's taking the Changing the above to |
| } else if (typeof child === 'object') { | ||
| newArray.push({ ...child }); |
There was a problem hiding this comment.
This is going to be problematic for properties that are Array. The individual array values will be spread as properties on the state.
There was a problem hiding this comment.
Ah, good catch. Do you think something like this would be okay?
| } else if (typeof child === 'object') { | |
| newArray.push({ ...child }); | |
| } else if (typeof child === 'object' && !Array.isArray(child)) { | |
| newArray.push({ ...child }); |
There was a problem hiding this comment.
yea, that could work.
But if it's just graphite maybe we can fix the mutation here: https://github.com/grafana/grafana/blob/6abe99efd64b8867112d9d9c74971d96840ce32d/public/app/plugins/datasource/graphite/state/store.ts#L53
(and elsewhere)
|
this bug was also reported for Google Cloud Monitoring datasource: grafana/grafana#97223 (comment) |
There was a problem hiding this comment.
Hey @kaydelaney 👋🏾 , as other mentioned, can we fix this issue in the problematic data sources that are doing the mutation? There might be more side effects on implementing this approach
|
Is there really more side effects to implementing this approach? If I were to clone a panel I wouldn't expect properties on it to be referring to the same property as the old panel, I think this has a great potential to cause more bugs in the future. That said, the mutation in the graphite DS is also not great and should probably be fixed too. |
| @@ -45,6 +45,8 @@ export function cloneSceneObjectState<TState extends SceneObjectState>( | |||
| for (const child of propValue) { | |||
| if (child instanceof SceneObjectBase) { | |||
| newArray.push(child.clone()); | |||
| } else if (typeof child === 'object' && !Array.isArray(child)) { | |||
| newArray.push({ ...child }); | |||
There was a problem hiding this comment.
An issue just popped up around transformations that is kind of related to this PR. Basically, duplicating a panel with some transformation and then modifying the transformation options within the second panel would also modify the transformation options in the initial panel.
This happens because of the spread operator won't cover objects that contain deeper nested arrays, which is the case for transformations where you'd have an object of the form:
{
id: "convertFieldType",
options: {
conversions: [{ ...some objects in array }]
}
}
Modifying this line to use lodash cloneDeep instead of spread op fixes the issue with transformations, so maybe we can use that?
There was a problem hiding this comment.
Thanks for the info! I worry that using cloneDeep might introduce some performance issues considering how often scene objects are cloned, but I'd need to benchmark it to be sure. I'll look into it :)
There was a problem hiding this comment.
Indeed, I share your worry, thought about it myself, not sure how to deal with it tho. Some benchmarking would be great!
Fixes an issue where if a panel was duplicated, both the original panel and duplicated panel's
queriesstate (in SceneQueryRunner) referred to the same object in memory, meaning when the duplicate query was changed, so was the original.Thread: https://raintank-corp.slack.com/archives/C03KVDHTWAH/p1730897481961219
📦 Published PR as canary version:
5.37.1--canary.967.12828986565.0✨ Test out this PR locally via: